packages/stack-cli/src/lib/iso.ts (1)

400-403: Consider adding error handling for writeFileSync.

The writeIso function directly calls writeFileSync without handling potential I/O errors. Per coding guidelines, avoid uncaught errors. Consider wrapping with appropriate error handling or documenting that callers must handle exceptions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-cli/src/lib/iso.ts` around lines 400 - 403, The writeIso
function calls writeFileSync directly and can throw uncaught I/O errors; wrap
the writeFileSync(path, buf) call in a try/catch inside writeIso and handle
errors by adding contextual information (include path and volumeId) and either
logging via the module/logger used in this package and rethrowing a new Error or
rethrowing the original error after attaching context, so callers get clear
failure details; keep buildIso(...) unchanged and ensure the catch preserves the
original stack (e.g., include err as cause or append err.message) so debugging
remains possible.

packages/stack-cli/src/commands/emulator.ts (2)

93-102: Validate the credentials payload instead of casting res.json().

Line 93 trusts the backend shape via a type assertion, so a malformed response becomes { project_id: undefined, ... } and the CLI will happily print/inject bad credentials. Parse unknown and assert the three required string fields before returning. As per coding guidelines, "Validate all assumptions either through the type system (preferred), assertions, or tests." and "Do NOT use as/any/type casts to bypass the type system unless explicitly asked by the user."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-cli/src/commands/emulator.ts` around lines 93 - 102, The code
currently casts res.json() to a credential shape which can hide malformed
responses; change to parse the JSON as unknown, then validate that the result is
an object and that data.project_id, data.publishable_client_key, and
data.secret_server_key exist and are strings (and non-empty if desired) before
returning them; replace the type assertion on the `data` variable with a runtime
type guard/assertion that throws a descriptive error if validation fails so the
function that calls this code (the code around the `data` constant and its
return) never returns undefined credentials.

---

57-69: Use a monotonic clock for elapsed-time calculations.

Both the internal-PCK wait loop and the download progress math are derived from Date.now(), which can jump with wall-clock changes and skew timeouts/ETA. performance.now() is the safer fit in both places. As per coding guidelines, "Don't use Date.now() for measuring elapsed real time. Instead use performance.now()"

Also applies to: 304-313

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-cli/src/commands/emulator.ts` around lines 57 - 69, The
readInternalPck() loop (and the download-progress ETA math elsewhere) currently
uses Date.now() for elapsed-time which can jump; replace those elapsed/timeout
calculations to use a monotonic clock via performance.now() (or Node's
perf_hooks.performance) instead: compute deadline as performance.now() +
timeoutMs and compare performance.now() in the loop, and update any ETA/progress
math that uses Date.now() to use performance.now() so timeout/duration
arithmetic stays stable; ensure units remain milliseconds and import or
reference the correct performance symbol used in this codebase.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

.github/workflows/qemu-emulator-build.yaml (2)

300-300: ⚠️ Potential issue | 🟡 Minor

Quote the serial-log path in the failure handler.

Unquoted $HOME still triggers SC2086 here. Use "$HOME/.stack/emulator/run/vm/serial.log" to avoid word-splitting and globbing.

Suggested change

-        run: tail -100 $HOME/.stack/emulator/run/vm/serial.log 2>/dev/null || true
+        run: tail -100 "$HOME/.stack/emulator/run/vm/serial.log" 2>/dev/null || true

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/qemu-emulator-build.yaml at line 300, The failure handler
uses an unquoted path in the tail command which triggers shellcheck SC2086;
update the run step string (the line containing the tail invocation) to quote
the expanded path, e.g. replace the unquoted
$HOME/.stack/emulator/run/vm/serial.log with
"$HOME/.stack/emulator/run/vm/serial.log" (and apply the same quoting to any
similar $HOME-based paths in that handler) so word-splitting and globbing are
avoided.

---

63-67: ⚠️ Potential issue | 🟠 Major

Install Node for the arm64 build path too.

The if: matrix.arch == 'amd64' guard leaves the arm64 build dependent on whatever Node happens to be preinstalled on the runner, even though Line 132 still runs node docker/local-emulator/generate-env-development.mjs for every matrix entry. That makes the arm64 path brittle as the runner image changes.

Suggested change

-      - uses: actions/setup-node@v4
-        if: matrix.arch == 'amd64'
+      - uses: actions/setup-node@v4
         with:
           node-version: 22
           cache: pnpm

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/qemu-emulator-build.yaml around lines 63 - 67, The
workflow only runs actions/setup-node@v4 when `if: matrix.arch == 'amd64'`,
leaving arm64 to rely on runner defaults while the job later runs `node
docker/local-emulator/generate-env-development.mjs`; remove or widen the `if`
guard so setup-node@v4 is executed for all matrix.arch entries (or add a
separate setup-node step for arm64) and keep `node-version: 22` and `cache:
pnpm` so both amd64 and arm64 use the pinned Node version and pnpm caching.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

.github/workflows/qemu-emulator-build.yaml (1)

216-237: Consider extracting the QEMU build into a composite action.

The QEMU cache restore + build-from-source logic is duplicated between the build and test jobs. Extracting it into a composite action or reusable workflow would reduce duplication and ensure both jobs stay in sync.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/qemu-emulator-build.yaml around lines 216 - 237, Extract
the duplicated QEMU restore+build steps into a reusable composite action (e.g.,
.github/actions/qemu-build) or a reusable workflow and replace the inline blocks
in both the build and test jobs with a single call; move the actions/cache@v4
restore step and the shell build script (the block under the "Build QEMU 10.2.2
from source" step that references /opt/qemu, key qemu-10.2.2-${{ runner.os
}}-${{ runner.arch }}-v1, and the configure/make/install sequence) into the
composite action, expose inputs for version, prefix, target-list and cache key,
and return/cache-hit as an output so both jobs can consume the same behavior
without duplication.

docker/server/entrypoint.sh (1)

156-159: Consider using null-delimited pipeline for robustness.

The find | xargs chain without -print0/-0 can fail on filenames containing spaces or newlines. While the built app tree is unlikely to have such filenames, using null-delimited handling would silence the shellcheck warning and be more robust.

♻️ Suggested improvement

-  unhandled_sentinels=$(find "$WORK_DIR/apps" -type f -exec grep -l "STACK_ENV_VAR_SENTINEL" {} + | \
-    xargs grep -h "STACK_ENV_VAR_SENTINEL" | \
+  unhandled_sentinels=$(find "$WORK_DIR/apps" -type f -print0 | \
+    xargs -0 grep -l "STACK_ENV_VAR_SENTINEL" 2>/dev/null | \
+    xargs grep -h "STACK_ENV_VAR_SENTINEL" 2>/dev/null | \
     grep -o "STACK_ENV_VAR_SENTINEL[A-Z_]*" | \
     sort -u | grep -v "^STACK_ENV_VAR_SENTINEL$")

Alternatively, keep the current approach if you're confident the build output won't contain problematic filenames.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docker/server/entrypoint.sh` around lines 156 - 159, The pipeline that builds
unhandled_sentinels using find/xargs/grep is unsafe for filenames with
spaces/newlines; update the commands that produce and consume file lists (the
find invocation used to populate unhandled_sentinels and the xargs usage) to use
null-delimited output/inputs (e.g., find ... -print0 and xargs -0 or use grep
-Z) so filenames are handled robustly, and preserve the subsequent grep/sort
-u/grep -v processing to produce the same STACK_ENV_VAR_SENTINEL list.

packages/stack-cli/src/commands/emulator.ts (4)

620-622: Voided promise in cleanup path.

Per coding guidelines, promises should not be voided with .catch(() => {}). While this is a best-effort cleanup before process.exit(), consider at minimum logging the error for debugging purposes.

♻️ Suggested fix

           console.log("\nStopping emulator...");
           runEmulator("stop")
-            .catch(() => { /* best-effort stop */ })
+            .catch((err) => {
+              // Best-effort stop — log but don't block exit
+              console.warn(`Warning: failed to stop emulator cleanly: ${err instanceof Error ? err.message : err}`);
+            })
             .finally(() => process.exit(exitCode));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-cli/src/commands/emulator.ts` around lines 620 - 622, The
cleanup call currently voids errors from runEmulator("stop") with .catch(() =>
{}); change this to surface at least the error via logging (e.g., use the
existing logger or console to log the error) so failures aren't silently ignored
while preserving the finally that calls process.exit(exitCode); locate the
runEmulator("stop") invocation in emulator.ts and replace the empty catch with a
handler that logs the caught error (including context like "emulator stop
failed") before allowing the finally block to run.

---

57-68: Use performance.now() for elapsed time measurement.

Per coding guidelines, Date.now() should not be used for measuring elapsed real time. This timeout logic should use performance.now() instead.

♻️ Suggested fix

 async function readInternalPck(timeoutMs = 60_000): Promise<string> {
   const path = internalPckPath();
-  const deadline = Date.now() + timeoutMs;
+  const deadline = performance.now() + timeoutMs;
   let delay = 250;
-  while (Date.now() < deadline) {
+  while (performance.now() < deadline) {
     if (existsSync(path)) {
       const contents = readFileSync(path, "utf-8").trim();
       if (contents) return contents;
     }
     await new Promise((r) => setTimeout(r, delay));
     delay = Math.min(delay * 2, 2000);
   }
   throw new CliError(`Timed out waiting for emulator internal publishable client key at ${path}`);
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-cli/src/commands/emulator.ts` around lines 57 - 68, In
readInternalPck replace use of Date.now() for deadline/loop checks with
performance.now(): compute deadline = performance.now() + timeoutMs and change
the while condition and any timestamps to compare performance.now() against the
deadline; update any related variables (e.g., the deadline calculation and loop
condition inside readInternalPck) and ensure the runtime provides performance
(import from 'perf_hooks' if needed) so elapsed-time measurement follows the
coding guideline.

---

174-174: Consider using STACK_ prefix for PORT_PREFIX.

Per coding guidelines, environment variables should be prefixed with STACK_ (or NEXT_PUBLIC_STACK_). The fallback to PORT_PREFIX without prefix may be intentional for backwards compatibility, but consider documenting this or migrating to STACK_PORT_PREFIX.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-cli/src/commands/emulator.ts` at line 174, The environment
variable lookup for portPrefix currently reads process.env.PORT_PREFIX and
process.env.NEXT_PUBLIC_STACK_PORT_PREFIX; update it to prefer the STACK_ names
by checking process.env.STACK_PORT_PREFIX then
process.env.NEXT_PUBLIC_STACK_PORT_PREFIX, optionally retaining
process.env.PORT_PREFIX only as a final fallback for backwards compatibility (or
remove it if migrating). Modify the initializer that sets portPrefix (the const
portPrefix assignment) to reflect the new precedence and add a brief inline
comment noting the compatibility fallback if you keep process.env.PORT_PREFIX.

---

304-312: Use performance.now() for download timing.

The download speed calculation uses Date.now() for elapsed time measurement, which should be performance.now() per coding guidelines.

♻️ Suggested fix

-  const startedAt = Date.now();
+  const startedAt = performance.now();
   let downloaded = 0;
   let lastRender = 0;

   const render = (final: boolean) => {
-    const now = Date.now();
+    const now = performance.now();
     if (!final && now - lastRender < 100) return;
     lastRender = now;
-    const elapsed = Math.max(0.001, (now - startedAt) / 1000);
+    const elapsed = Math.max(0.001, (now - startedAt) / 1000);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/stack-cli/src/commands/emulator.ts` around lines 304 - 312, The
timing logic in the download progress (startedAt, lastRender and render) uses
Date.now(); replace it with performance.now() so elapsed uses high-resolution
timing—set startedAt = performance.now(), compute now = performance.now() inside
render, keep the 100ms throttle and compute elapsed = Math.max(0.001, (now -
startedAt) / 1000). If this runs in Node and performance is not globally
available, import performance from 'perf_hooks' at the top of the module; update
references in the render function accordingly.

docker/local-emulator/qemu/run-emulator.sh (1)

690-700: Consider a more robust readiness check after rotation.

The sleep 1 workaround (line 694) addresses a race between supervisor sending SIGTERM and the old Node processes dying. This is pragmatic but could still race if the old processes take longer than 1 second to stop. A more robust approach would be to wait for the services to become unavailable first (indicating old processes stopped), then wait for them to become available again.

💡 Optional: Wait for services to go down before checking up

       if ! qga_trigger_fast_rotate; then
         warn "Failed to trigger rotate-secrets — falling back to cold boot."
         snapshot_fallback_to_cold_boot
         return
       fi

-      # Wait for the *new* backend (post-supervisor-restart) to actually be
-      # listening. all_ready may briefly return true against the OLD Node
-      # processes between when supervisor sends SIGTERM and when the children
-      # die; sleep a beat so we measure the real readiness.
-      sleep 1
+      # Wait for services to become unavailable (old processes stopping),
+      # then wait for them to come back up (new processes starting).
+      local down_deadline=$((SECONDS + 10))
+      while [ "$SECONDS" -lt "$down_deadline" ] && all_ready; do
+        sleep 0.2
+      done
       if ! wait_for_condition "rotated services" "$SNAPSHOT_READY_TIMEOUT" all_ready; then

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docker/local-emulator/qemu/run-emulator.sh` around lines 690 - 700, The
single-second sleep after supervisor rotation is racy; replace the blind sleep
with a two-step readiness check that first waits for services to become
unavailable (old processes stopped) and then waits for them to be available
again using wait_for_condition: call wait_for_condition (e.g., "services down")
with a short timeout polling !all_ready or an all_down helper until services are
confirmed down, then call wait_for_condition "rotated services"
"$SNAPSHOT_READY_TIMEOUT" all_ready as before; update the block around
wait_for_condition, all_ready, and SNAPSHOT_READY_TIMEOUT (and keep the existing
fallback to snapshot_fallback_to_cold_boot and tail_vm_logs) so we avoid the
race without a fixed sleep.